-
Notifications
You must be signed in to change notification settings - Fork 38.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
skip iptables sync if no endpoint changes #41223
Conversation
I think this one is cleaner than the checksum one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
pkg/proxy/iptables/proxier.go
Outdated
activeEndpoints[svcPort] = true | ||
} | ||
} | ||
// Remove endpoints missing from the update. | ||
// Gather stale connections to endpoints missing from the update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: suggest rewording to "Check stale connections against endpoints missing from the update"
removedEndpoints := getRemovedEndpoints(curEndpointIPs, newEndpoints) | ||
for _, ep := range removedEndpoints { | ||
staleConnections[endpointServicePair{endpoint: ep, servicePortName: svcPort}] = true | ||
} | ||
glog.V(3).Infof("Setting endpoints for %q to %+v", svcPort, newEndpoints) | ||
// Once the set operations using the list of ips are complete, build the list of endpoint infos | ||
proxier.endpointsMap[svcPort] = proxier.buildEndpointInfoList(portsToEndpoints[portname], newEndpoints) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment: The only significant diff is that we were only doing the work to buildEndpointInfoList
lazily, but that function only seems as expensive as flattenEndpointsInfo
anyway (O(endpoints)), so I don't think we're burning any more cpu.
// record endpoints of unactive service to stale connections | ||
for _, ep := range proxier.endpointsMap[svcPort] { | ||
staleConnections[endpointServicePair{endpoint: ep.ip, servicePortName: svcPort}] = true | ||
} | ||
|
||
glog.V(2).Infof("Removing endpoints for %q", svcPort) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment: this version seems cleaner since we're not deleting from the map we're iterating, and there's no residual state as we start with a fresh map everytime.
/approve |
88c1f2f
to
87fe4dc
Compare
Should we get this in and let it soak for some time before cherry-pick into 1.5? |
[APPROVALNOTIFIER] This PR is APPROVED The following people have approved this PR: bprashanth, freehan Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
adding label based on #41223 (review) |
Automatic merge from submit-queue (batch tested with PRs 41223, 40892, 41220, 41207, 41242) |
@freehan I've had #39484 sitting around since forever that @thockin hasn't gotten to. That adds a boatload of testcases. And one of those testcases catches an error in this PR, which is that healthcheck updates are not correctly added anymore because proxier.updateHealthCheckEntries() is no longer called for every svcPort in the new endpoints map too. I'm happy to fix that up in #39484 if you like, provided I can actually get somebody (@thockin ? @bprashanth ?) to review it. |
Thanks guys, I have been out for about 2 weeks (combo of vaca, work, sick) and am now trying to get all these PRs accounted for. Minhan can own the review. I have another set of PRs between me, @timothysc, and @danwinship which make sync be called at a max rate - all together these should be a nice improvement. Should we do this same transform over Service updates? |
@thockin yeah, we probably should do the same for Service updates, but it's not as big of an issue since Service updates are much less frequent than Endpoints. |
@dcbw I think that should not cause any problem at this point. Here is the theory:
Since OnEndpointsUpdate is called at least twice a second (Due to master election logic in kube-controller-manager and kube-scheduler), having a half second delay to update health check should not cause any problem, right? I intended to cherry-pick this into 1.5 to help some customers. I think #39484 is too big for cherry-pick. I can add a surgical fix to combine svcPorts from newEndpointMap and proxier.endpointsMap, and trigger updateHealthCheckEntries. For #39484, just need to rebase and delete the surgical fix. What do you think? |
Automatic merge from submit-queue fix healthcheck update problem introduced by #41223 ref: #41223 surgical fix for #41223 (comment)
Alternative to #41173
fixes: #26637
No need to checksum. Just compare endpoint maps.